Board config: timer config simplifications #13871

bkueng · 2020-01-06T18:10:28Z

Having done another PX4 board port, the most tedious, error-prone and redundant part I find is the timer configuration. This PR aims at improving that, with generality in mind.

Summary

A current timer + channel config looks like this:

	{
		.base = STM32_TIM1_BASE,
		.clock_register = STM32_RCC_APB2ENR,
		.clock_bit = RCC_APB2ENR_TIM1EN,
		.clock_freq = STM32_APB2_TIM1_CLKIN,
		.first_channel_index = 0,
		.last_channel_index = 3,
		.handler = io_timer_handler0,
		.vectorno =  STM32_IRQ_TIM1CC,
		.dshot = {
			.dma_base = STM32_DMA2_BASE,
			.dmamap = DMAMAP_TIM1_UP,
			.start_ccr_register = TIM_DMABASE_CCR1,
			.channels_number = 4u /* CCR1, CCR2, CCR3 and CCR4 */
		}
	},

	{
		.gpio_out = GPIO_TIM1_CH4OUT,
		.gpio_in  = GPIO_TIM1_CH4IN,
		.timer_index = 0,
		.timer_channel = 4,
		.ccr_offset = STM32_GTIM_CCR4_OFFSET,
		.masks  = GTIM_SR_CC4IF | GTIM_SR_CC4OF
	},

Accompanied with several defines in the header in the form #define GPIO_TIM12_CH2OUT /* PH9 T12C2 FMU8 */ GPIO_TIM12_CH2OUT_2.

The PR reduces all of it to 2 lines, one for the timer definition:

initIOTimer(Timer::Timer1, DMA{DMA::Index2, DMA::Stream5, DMA::Channel6}),

and one for the channel:

initIOTimerChannel(io_timers, {Timer::Timer1, Timer::Channel4}, {GPIO::PortE, GPIO::Pin14}),

Details

the 1. commit breaks the cyclic dependency between the io_timers_t and timer_io_channels_t data structures, and removes some fields that are not required or we can easily get at runtime.
general removal of the use of GPIO_GPIOx_OUTPUT, and usage of timer_io_channels instead. The data structure stays otherwise the same, but instead of explicitly setting each field, constexpr methods are used for initialization during build time. This allows to specify only the information that is really required (i.e. timer, channel and pin), while ending up with the same binary.
some architecture-specific mapping code is required to map from the specified configuration (i.e. Timer::Timer1 to the NuttX defines (the ones that are currently used).
requires C++14 for the constexpr code ( update C++ STD from c++11 to c++14 #13811)

Benefits

specify only the details that are required
while still allowing for full flexibility. The existing method of explicitly initializing everything could still be used. But customizations can also be done much nicer, and more specific, see v4 (pulldown, da38cd0) or crazyflie (different clock frequency, 4d609bd) for example.
complex compile-time checks are possible, for example ensuring that all channels of a certain timer are grouped together.

Drawbacks

even though the method is generic, and could be used for SPI for example, it cannot be used for the NuttX board.h config, as it requires C++. Overall I still think it's worth exploring a script-based generator (including for the NuttX defconfig).

Bugs

While working through this, I found 2 configuration bugs:

nxp/fmuk66-v3: the 2. timer has 3 channels associated not 2
crazyflie: TIM2 channels are not all grouped together. The current data structure does not allow for this. This issue is still unresolved and I see several options:
- use a bitfield instead of first_channel_index and last_channel_index, or remove the fields altogether. The io_timer_handler IRQ handler would become slightly slower.
- use a custom mixer to account for this (assuming we won't have more boards with that issue)
- add a board-specific channel reodering

@davids5 @dagar

davids5 · 2020-01-06T19:05:18Z

@bkueng - this looks interesting. I do not understand how the association of the IP blocks that are need to support a timer are going to be done.

Like where KINETIS_SIM_SCGC6 is the place that FTM0 on a K66 is enabled by what bit is not the same for an FTM3 etc.

Where are cascaded clocks handled? (see the IMX rt branch)

ISRs should be short as possible. So not calculating (or shifting) at run time is preferred. How will that be done?

dagar · 2020-01-07T16:05:39Z

crazyflie: TIM2 channels are not all grouped together. The current data structure does not allow for this. This issue is still unresolved and I see several options:

use a bitfield instead of first_channel_index and last_channel_index, or remove the fields altogether. The io_timer_handler IRQ handler would become slightly slower.

use a custom mixer to account for this (assuming we won't have more boards with that issue)

add a board-specific channel reodering

A custom mixer for the crazyflie sounds fine, it's already a weird target. Last I heard it's not even working in master (I don't have one to test).

davids5 · 2020-01-07T16:22:49Z

@bkueng - please have a look at the IMXRT and suggest how this can be applied to it.

bkueng · 2020-01-13T16:08:47Z

A custom mixer for the crazyflie sounds fine, it's already a weird target.

I've done that.

@bkueng - please have a look at the IMXRT and suggest how this can be applied to it.

@davids5 I have merged the RT branch and done the changes (not quite finished yet) in bkueng@d363393 and bkueng@b5b6826. The main difference is that the IOMUX PAD and GPIO port + pin are independent. The timer setup and config could also be more generalized (also with cascaded clocks if needed), but I did not look into that.
Let me know what you think.

ISRs should be short as possible. So not calculating (or shifting) at run time is preferred. How will that be done?

All ISR routines are unchanged in terms of runtime.

If everything looks good I'll continue and update all boards.

davids5 · 2020-01-13T16:36:18Z

A custom mixer for the crazyflie sounds fine, it's already a weird target.

I've done that.

@bkueng - please have a look at the IMXRT and suggest how this can be applied to it.

@davids5 I have merged the RT branch and done the changes (not quite finished yet) in bkueng@d363393 and bkueng@b5b6826. The main difference is that the IOMUX PAD and GPIO port + pin are independent. The timer setup and config could also be more generalized (also with cascaded clocks if needed), but I did not look into that.
Let me know what you think.

ISRs should be short as possible. So not calculating (or shifting) at run time is preferred. How will that be done?

All ISR routines are unchanged in terms of runtime.

If everything looks good I'll continue and update all boards.

@bkueng - can we have a review of this on the dev call or some other time this week?

bkueng · 2020-01-14T10:04:05Z

Sure

davids5 · 2020-01-27T14:36:30Z

@bkueng - Now that I understand the genius in this I have a great appreciation for the work you have done herein.

As discussed I will rebase and bring the RT in this week, then you can bring this in on top of it.

Thank you!

codecov · 2020-01-29T13:49:55Z

Codecov Report

Merging #13871 into master will decrease coverage by 0.29%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #13871     +/-   ##
=========================================
- Coverage   52.98%   52.69%   -0.3%     
=========================================
  Files         625      605     -20     
  Lines       52991    51256   -1735     
=========================================
- Hits        28079    27008   -1071     
+ Misses      24912    24248    -664

Flag	Coverage Δ
#mavsdk	`?`
#python	`100% <ø> (+96.93%)`	⬆️
#sitl_mission_FW	`100% <ø> (?)`
#sitl_mission_MC_box	`29.83% <100%> (?)`
#sitl_mission_MC_offboard_att	`100% <ø> (?)`
#sitl_mission_MC_offboard_pos	`28.56% <100%> (?)`
#sitl_mission_Rover	`26.3% <0%> (+0.94%)`	⬆️
#sitl_mission_VTOL_standard	`33.79% <100%> (?)`
#sitl_mission_VTOL_tailsitter	`33.82% <100%> (+0.99%)`	⬆️
#sitl_python_FW	`29.32% <0%> (+1.03%)`	⬆️
#sitl_python_MC_box	`100% <ø> (+70.93%)`	⬆️
#sitl_python_MC_offboard_att	`100% <ø> (+72.35%)`	⬆️
#sitl_python_MC_offboard_pos	`28.64% <100%> (+0.8%)`	⬆️
#sitl_python_Rover	`100% <ø> (?)`
#sitl_python_VTOL_standard	`33.86% <100%> (?)`
#sitl_python_VTOL_tailsitter	`100% <ø> (+67.1%)`	⬆️
#unittest	`36.98% <100%> (+2.97%)`	⬆️

Impacted Files	Coverage Δ
src/lib/mixer/MultirotorMixer/MultirotorMixer.cpp	`84.06% <100%> (ø)`	⬆️
src/modules/navigator/takeoff.cpp	`3.92% <0%> (-76.48%)`	⬇️
...flight_tasks/tasks/Failsafe/FlightTaskFailsafe.cpp	`0% <0%> (-73.92%)`	⬇️
src/modules/navigator/rtl.cpp	`14.46% <0%> (-44.22%)`	⬇️
...lib/flight_tasks/tasks/Manual/FlightTaskManual.cpp	`0% <0%> (-38.89%)`	⬇️
src/modules/navigator/loiter.cpp	`8.16% <0%> (-38.78%)`	⬇️
Tools/px4airframes/xmlout.py	`98.18% <0%> (ø)`	⬆️
src/lib/parameters/px4params/srcparser.py	`86.87% <0%> (ø)`	⬆️
src/modules/sensors/vehicle_imu/VehicleIMU.cpp	`43.87% <0%> (-33.4%)`	⬇️
src/modules/simulator/simulator_mavlink.cpp	`59.31% <0%> (-19.06%)`	⬇️
... and 97 more

bkueng · 2020-01-29T14:01:52Z

This is now getting ready, I updated all boards.
Tested so far on Pixracer, both PWM + DShot.

@davids5 once the imxrt is in, I'll rebase once again and update that as well.

@PX4/testflights can you test this on all boards that you have?

davids5 · 2020-01-29T14:48:40Z

@bkueng - So that I am clear, moving forward the defconfig will not state what HW Timers are enabled if PX4 is driving them as PWM ?

I am guessing it was inconsistent?

What about UAVCAN, HRT etc?

The thing we will need to look out for is a HW timer that has shared usage. Assume A and B a drivers use TIM1.
A & B had the timers enabled in the RCC before either driver runs and the start up ordered or A and B did not matter. Now all drivers need to do the ENR enable AND not use the RST in the rcc on a shared timer.

The former MAY manifest it's self as a hardfault (buss fault on not enabled IP block), the more subtle failure will be Wrong (or reset) values in registers.

We should leverage all the goodness you have added to get a compilation time HW resource usage/contention map.

bkueng · 2020-01-29T15:32:52Z

Yes it was inconsistent, and it was also never clear to me when I ported a board if it was needed/harmful/does not matter.

HRT also has a check (# error must not set CONFIG_STM32_TIM1=y and HRT_TIMER=1), for UAVCAN I don't know.
I agree we should consolidate that further, especially where overlapping use is possible.
From what I saw, we or NuttX does not init any timers by just enabling the NuttX timer config, but I may have overlooked something.

davids5 · 2020-01-29T16:05:49Z

@bkueng

not init any timers

That is true if the driver are not included via defconfig.

RCC is it. An example of the RCC stuff is here. is here. https://github.com/PX4/NuttX/blob/px4_firmware_nuttx-8.2/arch/arm/src/stm32/stm32f40xxx_rcc.c#L332-L386

Junkim3DR · 2020-01-29T17:54:15Z

Tested on NXP FMUK66 v3

Modes Tested

Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.
Mission Plan Mode (Automated): Good.
RTL (Return To Land): Good.

Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to altitude then stabilized mode proceed to switch to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoint then trigger RTL.

Notes
No issues noted, good flight in general.

Log

https://review.px4.io/plot_app?log=24dab5c2-ee6f-4e5a-aee9-a1436b7deb10

dagar · 2020-01-29T19:22:04Z

The NXP i.MXRT changes (#13678) have merged to master.

bkueng · 2020-01-31T10:10:08Z

Updated for imxrt - it is really not SW-developer friendly.
I validated that the generated timer/channel data structure is binary-equivalent to before, so I expect it to work.

Tony3dr · 2020-01-31T18:57:20Z

We are encountering issues with CUAV Nano V5 and Pixhawk 4 V5. The firmware is broken for those vehicles, no connection on the vehicles doesn't detect the ESC's, see image below:

@bkueng

jorge789 · 2020-01-31T20:12:01Z

Tested on PixRacer V4

Indoor test flight.
Mode tested on Stabilized

Takeoff and landing on Stabilized

Log: https://review.px4.io/plot_app?log=b797910e-f15b-433a-9b52-1167a364d0de

These are not required, and to be consistent we enforce disabling them now.

…nnel indexes IOMUX uses different enumeration from GPIO pin + port, so we cannot use .gpio_out, and add a .gpio_portpin.

davids5

@bkueng - I have not tested on RT, but that should not stop this from coming in,

dagar requested review from davids5 and dagar January 6, 2020 19:19

dagar added Admin: Enhancement (improvement) 💡 NuttX (OS) labels Jan 6, 2020

bkueng force-pushed the timer_config_refactoring branch from e98df6d to ebfb18c Compare January 13, 2020 15:56

bkueng force-pushed the timer_config_refactoring branch from ebfb18c to e48b618 Compare January 29, 2020 13:28

bkueng changed the title ~~[RFC] Board config: timer config simplifications~~ Board config: timer config simplifications Jan 29, 2020

bkueng force-pushed the timer_config_refactoring branch from a55e2af to 9fd53a6 Compare January 29, 2020 14:04

bkueng force-pushed the timer_config_refactoring branch from 9fd53a6 to 43a0d4c Compare January 29, 2020 15:33

bkueng force-pushed the timer_config_refactoring branch from 43a0d4c to e2f3065 Compare January 31, 2020 10:00

weekly-digest bot mentioned this pull request Feb 2, 2020

Weekly Digest (26 January, 2020 - 2 February, 2020) #14084

Closed

bkueng force-pushed the timer_config_refactoring branch from e2f3065 to e4d7166 Compare February 3, 2020 10:42

bkueng added 22 commits February 13, 2020 11:16

io-v2: use hw description methods for timer configuration

d457c63

stm32 io_timer: add support for 16 PWM outputs

d46aa03

airmind/mindpx-v2: use hw description methods for timer configuration

5b82f6b

av/x-v1: use hw description methods for timer configuration

b2f1e13

holybro/kakutef7: use hw description methods for timer configuration

a159661

intel/aerofc-v1: use hw description methods for timer configuration

84d7339

modalai/fc-v1: use hw description methods for timer configuration

c6b9c08

mro/ctrl-zero-f7: use hw description methods for timer configuration

2c4c98e

mro/x21-777: use hw description methods for timer configuration

9a275ef

mro/x21: use hw description methods for timer configuration

df08c2c

omnibus/f4sd: use hw description methods for timer configuration

5bc9eb4

px4/fmu-v2: use hw description methods for timer configuration

5356b0d

px4/fmu-v3: use hw description methods for timer configuration

00abc07

px4/fmu-v4pro: use hw description methods for timer configuration

51dadc1

px4/fmu-v5x: use hw description methods for timer configuration

6c1bd46

uvify/core: use hw description methods for timer configuration

ae1b456

nuttx configs: disable nuttx timers which are used for PWM output

7634dad

These are not required, and to be consistent we enforce disabling them now.

imxrt io_timer: remove some fields from io_timers_t and split out cha…

8b661ce

…nnel indexes IOMUX uses different enumeration from GPIO pin + port, so we cannot use .gpio_out, and add a .gpio_portpin.

px4_arch imxrt: add hw description constexpr util methods

6d5aceb

io_timer_init.h: avoid using timer_channel, as imxrt does not have it

e7ebea0

nxp/fmurt1062-v1: use hw description methods for timer configuration

90b3822

px4io: fix argv index & avoid empty 'unknown argument: ' warning

aa224a0

bkueng force-pushed the timer_config_refactoring branch from 4f9071d to aa224a0 Compare February 13, 2020 10:17

davids5 approved these changes Feb 13, 2020

View reviewed changes

davids5 merged commit 5e37963 into master Feb 13, 2020

davids5 deleted the timer_config_refactoring branch February 13, 2020 11:49

bkueng mentioned this pull request Feb 13, 2020

[RFC] Board config: SPI & I2C config simplifications #14156

Closed

weekly-digest bot mentioned this pull request Feb 16, 2020

Weekly Digest (9 February, 2020 - 16 February, 2020) #14167

Closed

mrpollo mentioned this pull request Mar 26, 2020

v1.11.0 Release Notes #14402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Board config: timer config simplifications #13871

Board config: timer config simplifications #13871

bkueng commented Jan 6, 2020

davids5 commented Jan 6, 2020

dagar commented Jan 7, 2020

davids5 commented Jan 7, 2020

bkueng commented Jan 13, 2020

davids5 commented Jan 13, 2020

bkueng commented Jan 14, 2020

davids5 commented Jan 27, 2020

codecov bot commented Jan 29, 2020 •

edited

Loading

bkueng commented Jan 29, 2020

davids5 commented Jan 29, 2020

bkueng commented Jan 29, 2020

davids5 commented Jan 29, 2020

Junkim3DR commented Jan 29, 2020

dagar commented Jan 29, 2020

bkueng commented Jan 31, 2020

Tony3dr commented Jan 31, 2020

jorge789 commented Jan 31, 2020

davids5 left a comment

Board config: timer config simplifications #13871

Board config: timer config simplifications #13871

Conversation

bkueng commented Jan 6, 2020

Summary

Details

Benefits

Drawbacks

Bugs

davids5 commented Jan 6, 2020

dagar commented Jan 7, 2020

davids5 commented Jan 7, 2020

bkueng commented Jan 13, 2020

davids5 commented Jan 13, 2020

bkueng commented Jan 14, 2020

davids5 commented Jan 27, 2020

codecov bot commented Jan 29, 2020 • edited Loading

Codecov Report

bkueng commented Jan 29, 2020

davids5 commented Jan 29, 2020

bkueng commented Jan 29, 2020

davids5 commented Jan 29, 2020

Junkim3DR commented Jan 29, 2020

Tested on NXP FMUK66 v3

dagar commented Jan 29, 2020

bkueng commented Jan 31, 2020

Tony3dr commented Jan 31, 2020

jorge789 commented Jan 31, 2020

Tested on PixRacer V4

davids5 left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 29, 2020 •

edited

Loading